Convert unit tests to Typescript #258
Conversation
Signed-off-by: Levko Kravets <[email protected]>
kravets-levko
requested review from
arikfr,
superdupershant,
yunbodeng-db,
andrefurlan-db,
rcypher-databricks,
jackyhu-db and
benc-db
as code owners
lib/DBSQLClient.ts
Outdated
| export default class DBSQLClient extends EventEmitter implements IDBSQLClient, IClientContext { | ||
| private static defaultLogger?: IDBSQLLogger; | ||
|
|
||
| private readonly config: ClientConfig; | ||
|
|
||
| private connectionProvider?: IConnectionProvider; | ||
| protected connectionProvider?: IConnectionProvider; |
There was a problem hiding this comment.
Ideally we test through the public interface. Do we need to expose these to get reasonable test coverage? I don't know the standards in the Node world, but in general my approach is, if there are methods that should be private, but I need to expose in order to test, I make a new type where those methods make sense as public methods, and have the original type call the new type to accomplish the work. This allows you to mock the new class in the tests of the original class, and not feel compelled to have a large public interface that consumers really shouldn't call. Given how big a change we are already doing, I won't block this PR on that, but it's something we should consider before we make another release of this library.
There was a problem hiding this comment.
Yes, I took another look at these changes and totally agree with you. I'll revert all similar changes (private -> protected) and temporarily just suppress TS errors with @ts-expect-error
There was a problem hiding this comment.
For now I kept code that uses private fields and methods - this PR is already too big to add more refactoring. However, it turns out that TS allows to access private and protected fields using string indexing (obj['prop']) - TS still checks if property exists and keeps things type-safe, so I think it's still better than untyped code or with errors suppressed
There was a problem hiding this comment.
I'm approving but given how big an interface change we have here, primarily to remove type errors from tests, please get another reviewer before merge and consider my comment about refactoring in a way that introduces new types, rather than modifying the interface of existing types.
Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: Levko Kravets <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
- Coverage 93.06% 92.93% -0.13%
==========================================
Files 65 64 -1
Lines 1586 1572 -14
Branches 280 282 +2
==========================================
- Hits 1476 1461 -15
+ Misses 46 44 -2
- Partials 64 67 +3 ☔ View full report in Codecov by Sentry. |
Part of PECO-1390
Initially I planned to convert unit tests gradually, but there were issues with collecting coverage data. So I had to convert everything at once